Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional serde support for option types and updates SpeechOptions to use Duration instead of millisecond u32 fields, aligning the crate’s configuration surface with more idiomatic Rust time handling.
Changes:
- Add
serdefeature (withhumantime-serde) and deriveSerialize/DeserializeforSessionOptions,SpeechOptions, andSampleRate. - Replace
SpeechOptionsduration fields/accessors/builders from*_ms: u32toDuration-based APIs. - Apply
cfg_attr(not(tarpaulin), inline(always))in a few hot/leaf methods and bump crate version + changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/options.rs | Introduces serde support and refactors SpeechOptions to Duration + adds serde-focused tests. |
| src/session.rs | Applies cfg_attr(not(tarpaulin), inline(always)) to several constructors/helpers. |
| src/detector.rs | Adapts to SpeechOptions no longer being Copy (uses .clone()) and minor inline attribute changes. |
| examples/streaming.rs | Adjusts example to clone SpeechOptions for reuse. |
| Cargo.toml | Bumps version to 0.2.0 and adds optional serde dependencies + feature. |
| CHANGELOG.md | Documents 0.2.0 changes (serde support + Duration migration). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl SessionOptions { | ||
| /// Create a new `SessionOptions` with default values. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn new() -> Self { | ||
| Self { | ||
| optimization_level: GraphOptimizationLevel::Disable, | ||
| optimization_level: GraphOptimizationLevel::Level3, | ||
| } | ||
| } |
There was a problem hiding this comment.
SessionOptions::new() sets optimization_level to Level3, but the serde default for a missing field is Disable and the unit test below expects SessionOptions::default() to be Disable. This inconsistency will fail tests and makes deserialization of {} not match SessionOptions::default(). Align these defaults (either revert new() to Disable or update the serde default + tests/docs to match the intended new default).
| /// Returns the minimum duration of detected speech segments, in milliseconds. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn min_speech_duration_ms(&self) -> u32 { | ||
| self.min_speech_duration_ms | ||
| pub const fn min_speech_duration(&self) -> Duration { | ||
| self.min_speech_duration | ||
| } | ||
|
|
||
| /// Returns the minimum duration of silence required to close a detected speech segment, in milliseconds. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn min_silence_duration_ms(&self) -> u32 { | ||
| self.min_silence_duration_ms | ||
| pub const fn min_silence_duration(&self) -> Duration { | ||
| self.min_silence_duration |
There was a problem hiding this comment.
These docs still say "in milliseconds" but the APIs now return Duration (and allow non-ms values when constructed programmatically / via serde). Update the wording to describe Duration (and, if you intentionally treat values as ms-precision, document the rounding/truncation behavior).
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
No description provided.